Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert allow to expect #402

Closed
wants to merge 5 commits into from
Closed

Conversation

KGrewal1
Copy link
Contributor

Start on converting allow to expect - avoided codegen / macros as in some cases makes sense (eg lint fires off in some code generated and not others), and avoided some lints such as Missing_Docs, where expect would say the lint is not needed but no allow resulted in the lint triggering

@KGrewal1 KGrewal1 requested a review from a team as a code owner November 26, 2024 17:29
@github-actions github-actions bot added the Guide label Nov 26, 2024
@KGrewal1
Copy link
Contributor Author

Start on Issue #300

@@ -370,7 +370,7 @@ pub(crate) mod serialized_request {
use serde::Deserializer;
use serde::Serializer;

#[allow(clippy::borrowed_box)] // required by serde
#[expect(clippy::borrowed_box, reason = "Required by serde")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: unless there's a strong existing convention in the community to capitalize, I'd default to keeping these reason strings lowercase

@@ -18,7 +18,7 @@ pub struct Options {
}

impl Options {
#[allow(clippy::expect_fun_call)]
#[expect(clippy::expect_fun_call)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't tell if this is even needed? I guess that's precisely what switching to expect will help us catch, ha. Lets see what CI comes back with...

@daprilik
Copy link
Contributor

Thanks for taking a crack at this! We'd love to see this land, though by the looks of it, there's still quite a few iterations left to go before all the lints get ironed out here.

We'll try to be reasonably timely with releasing CI runs here, but if we're lagging behind, you may want to look at the CI runner logs and figure out which cargo clippy invocations you can use to test this PR locally.

@KGrewal1
Copy link
Contributor Author

Resolved review comments - have clippy working for linux / macos builds fully now with

cargo clippy --locked --keep-going --tests --all-targets --workspace --features ci --target x86_64-unknown-linux-gnu --profile dev --exclude guest_test_uefi     

(think I was probably missing some targets / features prerviously), but don't have windows test for the same locally

@daprilik
Copy link
Contributor

I believe you should be able to clippy lint the windows build by specifying a custom target, and also ensuring that vars like MINIMAL_RT_BUILD and SPARSE_MMAP_NO_BUILD are set correctly? But if not... you may want to look into setting up some sort of working Windows / windows-cross environment for this sort of sweeping repo-wide lint work.

@KGrewal1
Copy link
Contributor Author

Going to close this for now then (as if I do get access to windows box, will need to push onto branch to sync so no point reopening until there is something finished) - apologies for opening too hastily!

@KGrewal1 KGrewal1 closed this Nov 28, 2024
@daprilik
Copy link
Contributor

No worries - thanks for taking a crack at it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants